Skip to content

tools: api_booster tool for upgrading Envoy APIs.#9258

Closed
htuch wants to merge 5 commits intoenvoyproxy:masterfrom
htuch:api-booster
Closed

tools: api_booster tool for upgrading Envoy APIs.#9258
htuch wants to merge 5 commits intoenvoyproxy:masterfrom
htuch:api-booster

Conversation

@htuch
Copy link
Member

@htuch htuch commented Dec 6, 2019

This is a beachhead PR for a Clang Libtooling based workflow that automagically updates Envoy's
source tree to the latest API version for every referenced package. So far, ths tool is only capable
of inferring types and performing header fixups, later PRs will expand this.

Risk level: Low
Testing: Manual cleanup of all headers in source/ test/ and include/, all tests pass.

Part of #8082

Signed-off-by: Harvey Tuch htuch@google.com

This is a beachhead PR for a Clang Libtooling based workflow that automagically updates Envoy's
source tree to the latest API version for every referenced package. So far, ths tool is only capable
of inferring types and performing header fixups, later PRs will expand this.

Risk level: Low
Testing: Manual cleanup of all headers in source/ test/ and include/, all tests pass.

Part of envoyproxy#8082

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Member Author

htuch commented Dec 6, 2019

@mattklein123 @lizan @derekargueta there are a few TODOs still in there, but this is basically the next step in the automagic updates. The meat of this PR is everything in tools/. The rest is automated application of these tools and one or two minor hand fixups (e.g. adding missing non-API header files or deps that were needed to run libtooling across the tree).

# Figure out where the LLVM include path is. We need to provide this
# explicitly as the api_booster is built inside the Bazel cache and doesn't
# know about this path.
# TODO(htuch): this is fragile and depends on Clang version, should figure out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what library you need from clang/9.0.0/include? The versioned include only have headers for compiler-rt, so I doubt you actually need this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't declare this as -isystem, I see:

...clang-9.0/lib/clang/9.0.0/include/mmintrin.h:50:12: error: use of old-style cast [-Werror,-Wold-style-cast]
    return (__m64)__builtin_ia32_vec_init_v2si(__i, 0);    

since it doesn't recognize these libraries as system libs (and hence subject to relaxation of the warning).

@@ -35,7 +35,38 @@ py_binary(
],
)

# Pack API type database text file into a char* string that can be referenced
# at the C++ level.
genrule(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we generate this cc in type_database rule in #9276?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a big win from this? Since this genrule just gets invoked on the artifact from type_database() and is only ever consumed in one place (api_type_db_lib), it's easier to read/maintain if this is outside of a Bazel rule and we just use stock genrule.

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Member Author

htuch commented Dec 12, 2019

@lizan this is ready for another review, thanks.

@htuch
Copy link
Member Author

htuch commented Dec 12, 2019

@lizan I've also split out the tool related changes to #9329 to simplify review. We can keep this PR for reference and I'll rebase it after #9329 merges. Marking as "waiting" until then.

@htuch htuch added the waiting label Dec 12, 2019
@htuch
Copy link
Member Author

htuch commented Dec 13, 2019

Closing this out now that #9329 has merged, to simplify the followup API booster application.

@htuch htuch closed this Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants